dfp: Insert correct preresolved hostname key in DNS cache by abeyad · Pull Request #30784 · envoyproxy/envoy

abeyad · 2023-11-08T21:00:08Z

Previously, preresolved hostnames didn't include the port in the host that is used as a key in the DNS cache. This behavior differed from the regular resolution path which always normalized the host (via DnsHostInfo::normalizeHostForDfp). There was also a bug in the DnsCacheImplTest.PreresolveSuccess test, where the preresolve hostnames config included the port in the socket address, which is not how it would normally be specified and masked this issue.

This PR fixes the problem by ensuring the preresolved hostnames have the host normalized (via DnsHostInfo::normalizeHostForDfp) before inserting into the DNS cache, just like the host is normalized before retrieving from the DNS cache. The test issue is also fixed so we can now verify that the correct cache key is being used for insertion, retrieval, and removal, and test coverage is added to ensure we can fetch the cache entry correctly whether the host includes the port or not.

Previously, preresolved hostnames didn't include the port in the `host` that is used as a key in the DNS cache. This behavior differed from the regular resolution path which always normalized the host (via DnsHostInfo::normalizeHostForDfp). There was also a bug in the DnsCacheImplTest.PreresolveSuccess test, where the preresolve hostnames config included the port in the socket address, which is not how it would normally be specified and masked this issue. This PR fixes the problem by ensuring the preresolved hostnames have the host normalized (via DnsHostInfo::normalizeHostForDfp) before inserting into the DNS cache, just like the host is normalized before retrieving from the DNS cache. The test issue is also fixed so we can now verify that the correct cache key is being used for insertion, retrieval, and removal, and test coverage is added to ensure we can fetch the cache entry correctly whether the host includes the port or not. Signed-off-by: Ali Beyad <abeyad@google.com>

RyanTheOptimist

Nice catch!

RyanTheOptimist · 2023-11-08T21:03:08Z

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

-  EXPECT_CALL(*resolver_, resolve("bar.baz.com", _, _))
+  std::string host = "bar.baz.com";
+  uint32_t port = 443;
+  std::string authority = host + ":" + std::to_string(port);


nit: I think absl::StrCat() is the canonical way to do this sort of concatenation.

yeah i was debating whether to do that and add the new include or just use + and to_string() (since I see test code that use these quite liberally), but i just changed it to StrCat

RyanTheOptimist · 2023-11-08T21:07:07Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -58,7 +58,10 @@ DnsCacheImpl::DnsCacheImpl(
    // cache to load an entry. Further if this particular resolution fails all the is lost is the


(I'm trying to comment on line 56)

Wow, is this the correct type for hostname?

repeated config.core.v3.SocketAddress preresolve_hostnames = 10;

Hostname is a bizarre term for this as it seems to be a protocol (tcp/udp), an IP address, a port, and a resolver name. Am I reading that correctly? bizarre

I was also surprised SocketAddress was used for preresolved hostnames config, and doesn't seem to be the right structure to represent a DNS cache entry. Changing it is a good idea but probably outside the scope of the PR. I do share your concern though..

RyanTheOptimist · 2023-11-08T21:07:54Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

    // this DNS cache asks for it.
-    startCacheLoad(hostname.address(), hostname.port_value(), false);
+    const std::string host =
+        DnsHostInfo::normalizeHostForDfp(hostname.address(), hostname.port_value());


Do we need a runtime guard for this, since it looks like a behavior change? (Or is there some reason we're safe here)

I was debating this myself, and could be convinced of adding a runtime guard. I just figured that no one is using this currently (preresolved hostnames config), and in any case, all that will happen if the cache key change causes a cache miss is that the DNS record will have to be fetched (which is actually what was happening previously on "production" use cases, i.e. my traffic director integration test, without us knowing about it, you can just tell from the logs).

I think I'd be inclined to add a guard (which will default to true) since it's easy to add and better safe than sorry. I think we know that nobody is using the mobile APIs for this config, but I'm not sure if we know about DFP users in the wild?

good point, i had assumed DFP is only used for mobile but didn't consider the possibility it's used outside of mobile. runtime guard has been added!

Signed-off-by: Ali Beyad <abeyad@google.com>

repokitteh-read-only · 2023-11-09T17:22:54Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #30784 was synchronize by abeyad.

see: more, trace.

RyanTheOptimist

Thanks for doing this!

abeyad · 2023-11-09T17:29:54Z

Adding @lizan as DFP owner and cross-company reviewer

abeyad · 2023-11-09T20:09:55Z

/retest

lizan

one style nit. otherwise LGTM. @RyanTheOptimist feel free to merge.

lizan · 2023-11-10T08:30:44Z

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

    config_.mutable_max_hosts()->set_value(max_hosts);
    if (!preresolve_hostnames.empty()) {
-      for (const auto& hostname : preresolve_hostnames) {
+      for (const auto& host_pair : preresolve_hostnames) {


nit:

Suggested change

for (const auto& host_pair : preresolve_hostnames) {

for (const auto& [host, port] : preresolve_hostnames) {

Thanks for the recommendation, done!

Signed-off-by: Ali Beyad <abeyad@google.com>

RyanTheOptimist · 2023-11-10T16:40:15Z

I think the protobuf check will fail until #30826 lands and is merged into this PR :(

abeyad · 2023-11-10T19:34:30Z

/retest

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2023-11-10T21:51:03Z

/retest

abeyad assigned RyanTheOptimist Nov 8, 2023

abeyad requested review from alyssawilk and mattklein123 as code owners November 8, 2023 21:00

RyanTheOptimist reviewed Nov 8, 2023

View reviewed changes

abeyad added 2 commits November 8, 2023 21:48

strcat

921fd80

Signed-off-by: Ali Beyad <abeyad@google.com>

add runtime guard

4b98e17

Signed-off-by: Ali Beyad <abeyad@google.com>

RyanTheOptimist previously approved these changes Nov 9, 2023

View reviewed changes

abeyad assigned lizan Nov 9, 2023

lizan reviewed Nov 10, 2023

View reviewed changes

lizan feedback

26b0978

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad dismissed RyanTheOptimist’s stale review via 26b0978 November 10, 2023 15:42

abeyad enabled auto-merge (squash) November 10, 2023 15:43

RyanTheOptimist approved these changes Nov 10, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into dfp_dns_cache

97da9ef

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad merged commit 32e8b48 into envoyproxy:main Nov 10, 2023

abeyad deleted the dfp_dns_cache branch November 10, 2023 22:08

alyssawilk mentioned this pull request Jul 23, 2024

envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns deprecation #35372

Closed

		@@ -58,7 +58,10 @@ DnsCacheImpl::DnsCacheImpl(
		// cache to load an entry. Further if this particular resolution fails all the is lost is the

	for (const auto& host_pair : preresolve_hostnames) {
	for (const auto& [host, port] : preresolve_hostnames) {

Comments

Conversation

abeyad commented Nov 8, 2023

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Nov 9, 2023

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

abeyad commented Nov 9, 2023

Uh oh!

abeyad commented Nov 9, 2023

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Nov 10, 2023

Uh oh!

abeyad commented Nov 10, 2023

Uh oh!

abeyad commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants